fix(listing): pick exact or create new one on update #726
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying datachain-documentation with
|
| Latest commit: |
3bcaa78
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://94f10ba5.datachain-documentation.pages.dev |
| Branch Preview URL: | https://fix-listing-selection.datachain-documentation.pages.dev |
shcheklein
force-pushed
the
fix-listing-selection
branch
from
50cf27d to
08e1e75
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #726 +/- ##
==========================================
- Coverage 87.43% 87.43% -0.01%
==========================================
Files 114 114
Lines 10967 10965 -2
Branches 1508 1507 -1
==========================================
- Hits 9589 9587 -2
Misses 998 998
Partials 380 380
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
shcheklein
force-pushed
the
fix-listing-selection
branch
from
08e1e75 to
73b68b9
Compare
shcheklein
force-pushed
the
fix-listing-selection
branch
from
73b68b9 to
4f66965
Compare
shcheklein
commented
Dec 21, 2024
shcheklein
force-pushed
the
fix-listing-selection
branch
from
4f66965 to
8ffe1d4
Compare
shcheklein
commented
Dec 21, 2024
shcheklein
force-pushed
the
fix-listing-selection
branch
from
8ffe1d4 to
f4b023c
Compare
shcheklein
commented
Dec 21, 2024
src/datachain/lib/dc.py
Outdated
| if listings and not update: | ||
| listing = sorted(listings, key=lambda ls: ls.created_at)[-1] | ||
|
|
||
| # For local file system we need to fix listing path / prefix |
There was a problem hiding this comment.
@ilongin Q: why do we need to do this only for the local file system?
shcheklein
changed the title
shcheklein
commented
Dec 21, 2024
| @@ -38,20 +39,43 @@ def _tree_to_entries(tree: dict, path=""): | |||
| @pytest.fixture | |||
| def listing(test_session): | |||
| catalog = test_session.catalog | |||
| dataset_name, _, _, _ = DataChain.parse_uri("s3://whatever", test_session) | |||
| dataset_name, _, _, _ = DataChain.parse_uri("file:///whatever", test_session) | |||
There was a problem hiding this comment.
[C]: minor semi-related change - there is not reason to use s3, tests were failing since it was hitting actual token update w/o proper fixtures. cc @ilongin
shcheklein
force-pushed
the
fix-listing-selection
branch
from
f4b023c to
3bcaa78
Compare
amritghimire
approved these changes
Dec 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partially addresses and stabilizes #725
Fixes a few situations in the way we find and suggest an existing cached listing when we do an
update=True.1. There was a bug in the origin code:
This code actually was picking the "biggest" one instead. There should have been
reverse=True.So, in a situation like:
file:///whatever/dir1exists and cachedfile:///whatever/exists and cachedand we were trying to update the
file:///whatever/dir1- it was updating thefile:///whatever/instead.2. Then, in a situation like:
file:///whatever/exists and cachedfile:///whatever/dir1doesn't yet existand we do "update=True" for the
file:///whatever/dir1it was updating the whole
file:///whatever/which can be arbitrary large. And we don't want to do this.So, bottom line is:
Generalizing a bit code for both of those cases led to the code in this PR.